Fix and forward gpepIncomingMessageHandler for Green Power support#713
Fix and forward gpepIncomingMessageHandler for Green Power support#713
Conversation
Real Green Power frames emitted by a Friends of Hue class switch never reached the host: bellows dropped them with 'Data is too short' at deserialization time, as reported in zigpy/zigpy#1814 with a capture from a Busch-Jaeger 6716 U switch on a Silabs ZBT-1 stick. Three things were wrong: - The v4 schema for this callback, inherited up to v16, treated the GP address as five scattered fields (addrType, addr:uint32, applicationId, address:EUI64, endpoint). The NCP actually sends a single 10-byte EmberGpAddress struct: applicationId, an 8-byte id union, and endpoint. The existing EmberGpAddress type also assumed the wrong layout (14 bytes with separate gpdIeeeAddress and sourceId fields). - The v17 override uses sl_GpStatus, a strict enum that only accepts 0x00..0x07. Current ZBT-1 firmware returns higher status bytes for frames that were not matched in the proxy table (0x7C observed on the captured frame), which dropped the whole callback before the address was even read. - EZSP v16 appends an SlRxPacketInfo struct after the LVBytes payload. zigbee-herdsman's ember adapter gates the read on 'version >= 0x10', so v13 and v14 do not carry this trailer but v16 does. The previous override that flowed through from v13 was one field short on v16. Fix EmberGpAddress to the correct 10-byte layout, add a v13 override of gpepIncomingMessageHandler that uses a plain uint8_t for status so unexpected values reach the host, and add a dedicated v16 override that appends the SlRxPacketInfo trailer. v14 picks the v13 override up through the _REPLACEMENTS loop. v17+ keep their own schema which now also benefits from the struct fix. Add tests driven by the exact bytes captured by the community tester, a smoke test on v14 to verify the schema flows through inheritance, and a v16 test that appends a synthetic SlRxPacketInfo to the same capture to exercise the trailer parsing.
Decoding the callback is only half the job: zigpy's GreenPower manager listens for GP frames through ControllerApplication. packet_received() on endpoint 242, cluster 0x0021, and expects a ZCL GP Notification (server command 0x00). Until now bellows dispatched nothing at all for this callback, so real GPDFs from a Friends of Hue switch like the Busch-Jaeger 6716 U never reached the host stack. Add a dedicated branch in ezsp_callback_handler that reencapsulates the flattened GPDF as a ZCL GP Notification and calls packet_received, mirroring what zigbee-herdsman's ember adapter does. Key behaviour: - SrcID-mode GPDs (applicationId == 0) are forwarded. IEEE-mode GPDs are dropped with a debug log because the current zigpy GP stack explicitly does not support them. - On EZSP v16+ the trailing SlRxPacketInfo supplies the proxy NWK, LQI and RSSI. On v13/v14 we fall back to the coordinator short address and the gpdLink field, since no proxy info is on the wire. - Malformed callbacks (short arg tuple) are dropped silently so a firmware mismatch cannot crash the dispatcher. - A new green_power_rx counter tracks how many GPDFs the NCP has handed us, mirroring the existing rx counters. Add tests that cover the v13/v14 shape, the v16 shape with SlRxPacketInfo, an operational data command (Toggle), the IEEE drop path, and the short-args drop path. Each test asserts the ZCL envelope, the NotificationSchema round-trip, and the packet routing fields zigpy needs.
Two CI failures on this PR: 1. pytest on Python 3.14 (and all other versions) fails with `AttributeError: module 'zigpy.zgp.types' has no attribute 'GP_ENDPOINT'`. These constants (``GP_ENDPOINT``, ``GP_CLUSTER_ID``) are not exported by the current zigpy release -- they would be added by zigpy/zigpy#1814, which is still unreleased. Depending on them here introduces a hard coupling to an unreleased upstream. 2. pre-commit (black) rewrites the aligned hex-dump comments in the BJ6716U capture and the split `bellows.ezsp.v13.commands.COMMANDS` lookup. Fix both by: - Defining ``GP_ENDPOINT`` (242), ``GP_CLUSTER_ID`` (0x0021) and ``GP_PROFILE_ID`` (0xA1E0) as module-level constants in ``bellows.zigbee.application``, citing the ZGP 1.1b profile. These values are stable and do not need to travel across repos. - Dropping the now-unused ``zigpy.zgp.types`` import from ``tests/test_application.py`` and replacing the assertions with the same literals. - Running black to compact the inline comments so pre-commit is idempotent. No behaviour change. All 22 EZSP v13/v14/v16 tests pass against a venv with PyPI zigpy (which does not ship GP_ENDPOINT), confirming the fix removes the upstream dependency.
If this PR does not break stuff if merged before zigpy/zigpy#1814 is merged into zigpy then suggest merge #713 first. Or? As far as I understand the normal order is to get upstream patches merge first, and technically bellows is upstream of zigpy. That is, if nothing breaks then there should be no need for both libraries can ship together, instead updates to bellows (and other radio libraries for zigpy too) can be reviewed, merged, and released directly as long as there no breaking changes. Or? |
|
Good point, and I agree. You're right that the type coupling was telling a different story. The previous version of this PR imported |
So maybe change this from a draft PR to ready for review, unless not ready yet? |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## dev #713 +/- ##
=======================================
Coverage 99.54% 99.54%
=======================================
Files 61 61
Lines 4147 4183 +36
=======================================
+ Hits 4128 4164 +36
Misses 19 19 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
codecov/patch flagged one uncovered line in bellows/types/struct.py (line 395, the gpd_ieee_address accessor). IEEE-addressed GPDFs are dropped upstream of this property in `_handle_gp_frame`, so no integration path reaches it. Add a direct unit test on the struct, plus a symmetric test for source_id so both accessor paths are exercised explicitly.
ZGP pairing flow: field test report (Busch-Jaeger 6716 U on HA 2026.4)Context
TL;DRThe EZSP -> bellows -> zigpy forwarding path is working end-to-end. The blocker is one layer above: the ZGP commissioning window on the sink is never opened. What works
To be testedZGP frames with a legacy zigpy version should be silently dropped. This would match what happened with recent versions of the EZSP protocol already handling ZGP frames. |
There was a problem hiding this comment.
Pull request overview
This PR updates bellows’ Green Power (GP) handling so gpepIncomingMessageHandler frames (EZSP callback 0x00C5) parse correctly on newer EZSP versions and are forwarded into zigpy’s Green Power pipeline as a ZCL GP Notification.
Changes:
- Fix GP address parsing by redefining
EmberGpAddressas the correct 10-byte on-wire layout and adding helper accessors. - Override
gpepIncomingMessageHandlercallback schemas for EZSP v13 (status asuint8_t) and v16 (adds trailingSlRxPacketInfo) and add regression tests based on a real capture. - Add callback forwarding in
ControllerApplication.ezsp_callback_handler()to re-encapsulate incoming GPDFs as ZCL GP Notifications on endpoint 242 / cluster 0x0021, plus tests.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
bellows/types/struct.py |
Redefines EmberGpAddress to match the real 10-byte wire format; adds source_id / gpd_ieee_address helpers. |
bellows/ezsp/v13/commands.py |
Overrides gpepIncomingMessageHandler schema to use EmberGpAddress and tolerate unknown status bytes. |
bellows/ezsp/v16/commands.py |
Overrides gpepIncomingMessageHandler schema to include the v16+ trailing SlRxPacketInfo. |
bellows/zigbee/application.py |
Adds forwarding of gpepIncomingMessageHandler to zigpy via a synthetic ZCL GP Notification and introduces a green_power_rx counter. |
tests/test_ezsp_v13.py |
Adds schema-level and protocol-level parsing tests using the real captured payload. |
tests/test_ezsp_v14.py |
Verifies v14 inherits the v13 schema override via replacements. |
tests/test_ezsp_v16.py |
Verifies v16 parsing with the appended SlRxPacketInfo trailer. |
tests/test_application.py |
Adds forwarding tests ensuring the GP callback becomes a zigpy packet_received() GP Notification and covers edge cases. |
tests/test_types.py |
Adds unit tests for the new EmberGpAddress helper properties. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if len(args) < 13: | ||
| LOGGER.debug("gpepIncomingMessageHandler: short args %r, dropping", args) | ||
| return | ||
|
|
||
| ( | ||
| _status, | ||
| gpd_link, | ||
| sequence_number, | ||
| addr, | ||
| gpdf_security_level, | ||
| gpdf_security_key_type, | ||
| _auto_commissioning, | ||
| _bidirectional_info, | ||
| gpd_security_frame_counter, | ||
| gpd_command_id, | ||
| _mic, | ||
| _proxy_table_index, | ||
| gpd_command_payload, | ||
| *rest, | ||
| ) = args | ||
|
|
||
| # v16+ appends an SlRxPacketInfo after the payload. Pull the | ||
| # proxy NWK out of it if it is there, otherwise fall back to the | ||
| # coordinator short address (the coordinator acts as the proxy). | ||
| packet_info = rest[0] if rest else None | ||
|
|
||
| if addr.applicationId == zgp_t.ApplicationID.SrcID: | ||
| source_id = addr.source_id | ||
| else: | ||
| LOGGER.debug( |
There was a problem hiding this comment.
_handle_gp_frame() assumes the v13+ callback layout (where args[3] is an EmberGpAddress). On older EZSP versions (e.g. v4 schema), gpepIncomingMessageHandler’s 4th argument is addrType (a uint8_t), so addr.applicationId will raise AttributeError and can crash the callback dispatcher. Add a version/shape guard (e.g. isinstance(addr, t.EmberGpAddress)) and either translate the legacy layout into an EmberGpAddress or drop/log the frame safely.
There was a problem hiding this comment.
Taken into account in 2df0ea5
Added isinstance(addr, t.EmberGpAddress) guard with a debug log and a regression test
| @@ -90,6 +90,42 @@ | |||
| "status": t.sl_Status, | |||
| }, | |||
| ), | |||
There was a problem hiding this comment.
These verbose Claude comments are not useful, please remove them.
| @@ -357,16 +357,43 @@ class EmberTokTypeStackZllSecurity(EzspStruct): | |||
|
|
|||
|
|
|||
| class EmberGpAddress(EzspStruct): | |||
There was a problem hiding this comment.
The SDK source shows the definition as:
/**
* @brief GPD Address for sending and receiving a GPDF.
*/
typedef struct {
union {
/** The IEEE address is used when the application identifier is
* ::SL_ZIGBEE_GP_APPLICATION_IEEE_ADDRESS.
*/
sl_802154_long_addr_t gpdIeeeAddress;
/** The 32-bit source identifier is used when the application identifier is
* ::SL_ZIGBEE_GP_APPLICATION_SOURCE_ID.
*/
sl_zigbee_gp_source_id_t sourceId;
} id;
/** Application identifier of the GPD. */
sl_zigbee_gp_application_id_t applicationId;
/** Application endpoint , only used when application identifier is
* ::SL_ZIGBEE_GP_APPLICATION_IEEE_ADDRESS
*/
uint8_t endpoint;
} sl_zigbee_gp_address_t;This differs from the field order in this struct.
There was a problem hiding this comment.
I tried matching the SDK order but the captured frame from @dmatscheko test fails to parse: the EZSP wire layout serializes applicationId first, even though the C struct declares id first.
| rssi = int(packet_info.last_hop_rssi) | ||
| else: | ||
| proxy_nwk = int(self.state.node_info.nwk) | ||
| lqi = int(gpd_link) |
There was a problem hiding this comment.
This is the v13/v14 fallback when no SlRxPacketInfo trailer is present.
Would you rather drop a synthetic LQI/RSSI on v13/v14 (e.g. lqi=0, rssi=0) ?
Read the 8-byte union directly at the call sites instead of going through the source_id / gpd_ieee_address properties. The class docstring also points out that the EZSP wire layout serializes applicationId first, even though the C SDK union puts id first.
Build the ZCL header through foundation.ZCLHeader.cluster instead of a magic byte literal, drop the verbose docstring, and bail out on EZSP < v13 callbacks where the 4th argument is a legacy uint8_t addrType instead of an EmberGpAddress.
Build the expected ZigbeePacket up front and compare with == instead of checking each field one by one.
| "mic": t.uint32_t, | ||
| "proxyTableIndex": t.uint8_t, | ||
| "gpdCommandPayload": t.LVBytes, | ||
| "packetInfo": t.SlRxPacketInfo, |
There was a problem hiding this comment.
I've tested this with an adapter running EZSPv13 and EZSPv18 and parsing fails because packetInfo is not actually part of the command payload.
Summary
Supports zigpy/zigpy#1814 (Zigbee Green Power in zigpy). Real GP frames from a Friends of Hue switch (Busch-Jaeger 6716 U on a ZBT-1 stick, reported by @dmatscheko) never reached the host today for two reasons:
gpepIncomingMessageHandlercallback schema, inherited from v4 up to v16, did not match the wire format.EmberGpAddressin particular was declared as 14 bytes of scattered fields instead of the real 10-byteapplicationId + id[8] + endpoint, and the strictsl_GpStatusenum rejected the higher status bytes current firmware returns for unmatched frames (0x7C observed in the capture).Issues
Reference Issue
#712
Should also resolve
#110
#229
What this PR does
Fix the callback parsing on EZSP v13 / v14 / v16
EmberGpAddress: 10-byte layout with.source_id/.gpd_ieee_addresshelpers.gpepIncomingMessageHandleron v13 withstatus: uint8_tso unexpected bytes reach the host; v14 picks it up through_REPLACEMENTS.SlRxPacketInfotrailer (zigbee-herdsman gates the read onversion >= 0x10).Forward the decoded callback to zigpy
gpepIncomingMessageHandlerbranch inezsp_callback_handlerthat reencapsulates the flattened GPDF as a ZCL GP Notification on endpoint 242 / cluster 0x0021, mirroring the ember adapter in zigbee-herdsman.SlRxPacketInfosupplies the proxy NWK, LQI and RSSI. On v13 / v14 the coordinator short address is used as a fallback.green_power_rxcounter to track how many GPDFs the NCP has handed us.Tests
_REPLACEMENTS, v16 with the trailer appended).packet_receivedand let the frame actually reach zigpy'sGreenPowerManager. Two paths covered: Toggle on a pre-registered device firesgp_command_received; NoSecurity commissioning registers the device viagp_device_joinedand schedules a GP Pairing.All tests pass locally (402 / 402, excluding 3 unrelated
test_ashtiming failures pre-existing ondev).Caveats
Test plan
pytest tests/green locallyruff checkno new errors